Added Convert Function as discussed on Issue#22#33
Added Convert Function as discussed on Issue#22#33heisenbuug wants to merge 12 commits intomlpack:masterfrom
Conversation
|
Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:
Thank you again for your contributions! 👍 |
kartikdutt18
left a comment
kartikdutt18
left a comment
There was a problem hiding this comment.
Hey @heisenbuug, Thanks a lot for working on this. I've left some style comments other than that it looks good to me.
Could you also add tests for this, we can download files from the net during runtime using utils in models repo (Refer other tests). Again, Thanks for working on this. 👍
Co-authored-by: kartikdutt18 <39593019+kartikdutt18@users.noreply.github.com>
kartikdutt18
added
t: added feature
s: needs review
and removed
s: needs review
s: unanswered
s: unlabeled
labels
|
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍 |
|
Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:
Thank you again for your contributions! 👍 |
There was a problem hiding this comment.
Hey @heisenbuug, Sorry I forgot about this PR. This looks promising, Let me know if you need any assistance. Thanks for this PR.
| ctr++; | ||
| } | ||
|
|
||
| auto create_JSON(std::vector<std::string>& tags, std::vector<std::string> rows) |
There was a problem hiding this comment.
Hey, can we use camel case to be consistent.
There was a problem hiding this comment.
@heisenbuug, Can I do it for you?
| auto tokenize(std::string& line) | ||
| { | ||
| std::vector<std::string> col_names; | ||
| tokenizer<escaped_list_separator<char> > tk(line, escaped_list_separator<char>()); | ||
| for (tokenizer<escaped_list_separator<char> >::iterator i(tk.begin()); i != tk.end(); ++i) | ||
| col_names.push_back(*i); | ||
| return col_names; | ||
| } | ||
|
|
||
| auto create_XML(std::vector<std::string>& tags, std::vector<std::string> rows) |
There was a problem hiding this comment.
I think there is some issue with indentation. Also, can we replace auto with data type (makes it easier to figure out all properties).
There was a problem hiding this comment.
@heisenbuug, Can I also do this?
| using namespace boost::property_tree; | ||
| using namespace boost; | ||
|
|
||
| class Convert |
There was a problem hiding this comment.
Hey, can we also add class description and usage here.
| } | ||
|
|
||
| public: | ||
| void convert(std::string path, std::string to) |
There was a problem hiding this comment.
Let's make this function static. No need to declare an object for converting datasets. Also Capitalize the first letter of the function.
|
Hey @kartikdutt18, I will add json. xml -> csv by the end of today or tomorrow... |
|
Hey @kartikdutt18, I noticed something... Shouldn't we have to put all of them in 1 file? Also, we are planning to reduce boost dependencies, right? So should we use boost here? |
3 participants
Issue link
Added a new folder dataset_utils and currently the convert function can convert